-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Find Unused Step Definitions command #8
Find Unused Step Definitions command #8
Conversation
Pardon my ignorance of VS Extensions... I have this new command running in Unit tests. But I can't figure out how to launch a second instance of VS that would use my version of the extension so that it can be tested via the UI. What's the procedure? |
Trying things out in VS:
|
Comments on the code:
|
One more dev note: As compiling the "package" project is pretty slow (as it needs to install the package to the exp hive), during development I usually select the "TestDebug" configuration (instead of "Debug") in VS. That does not include the package project to the build by default, so I can work with unit and specs tests much faster. If I want to try this out in VS, I just right click on the package project in the solution explorer and invoke build from there - that builds it even though it is not included in the configuration. |
The It is an interesting question though what the expected behavior is for this two key cases: Case 1
Shall we list WhenX twice (as an unused Given and an unused When) or only once? My vote would be probably to list it twice... but I'm not fully sure. (Of course the same problem can happen with two Whens ( Case 2
Shall we list WhenX as unused? Because the method itself is used (as When) but not used as Given. My vote would be probably to list it... but I'm not fully sure. (Of course the same problem can happen with two Whens ( |
I like the concept of "draft pull requests" quite much - they give you early indication of the integration without looking to be "ready" |
@clrudolphi could you please do a dummy push to this to see if the CI works better now with external PRs? |
Meaning you want me to execute |
yes |
d91addf
to
df8e7a7
Compare
Executed. |
I see in the vsct file that keyboard shortcuts are defined for each menu command. Should one be defined for this new command, and what should the keybinding be? |
@clrudolphi The |
This commit duplicates code from FindStepDefinitionUsageCommand. Not fully tested, but basically working.
@gasparnagy Thanks for the help so far. I have this wired in and working. Here is an update on status: First, an observation: when a Reqnroll solution is first loaded, the context menu items for 'Find Step Definition Usages' and this new one 'Find Unused Step Definitions' fail. The output shows:
Subsequent invocations do work properly. Am I doing anything incorrectly or is this known/expected behavior? Secondly, I need some guidance on what we want to be displayed in the dynamically built result list in the context menu. Taking a cue from the 'Find Step Definition Usages' command, I have it currently set up to show the Location and Method. For a step definition like this: The context menu is populated with this info: Per our previous dialog, it may be more helpful to include both the methodName and the RnR Attribute in that message. What are your thoughts? My next step is to create Specs to further test this. |
df8e7a7
to
54bfb4f
Compare
I have also observed this, but haven't got time to debug it yet (it is since a last change and somehow related to the async handling, I think). Feel free to ignore it for now.
This is a good question. So basically we have the following information that might be relevant:
For the most of the cases, the unused method will not have multiple attribute, so IMHO we should find a solution that is somewhat optimized for that, but it also works for the multi-attribute model as well. Maybe:
or
but this is probably too long (would need to check how it looks). (We can consider to add G/W/T as icon in addition to that, but I'm not sure.) |
Technical side-note: in the PRs it is better not to rewrite the commits (amend/force push), so that reviewers can better see what has been reviewed and what has changed since. At the end we will anyway squash the changes, so the individual commits will not be relevant. |
And BTW: it is fantastic that two days after someone bought up an idea, we have a working solution. Congrats! |
Sorry about that. My git setup has something weird about it. Sometimes when I go to Push, git thinks that I have conflicting commits from Origin even though I was the last one to commit and push. Perhaps it's me doing something wrong with my manner of using git (sometimes from the command line, sometimes from within VS, sometimes by using GH Desktop). |
Do we have available a large RnR project that this could be tested on? I'm curious about the performance and how long the user will have to wait for the context menu to be populated with the results. Since this is a result of a bunch of set subtraction operations we don't have a convenient means of progressively populating the result. On large solutions, this command might take a while. |
With this command line tool, you can generate a big sample project. https://github.com/reqnroll/Reqnroll.VisualStudio/tree/main/Tests/Reqnroll.SampleProjectGenerator I think for many users, the version integrated to vs is beneficial. Having it as a ci option might also make sense, but that is another story - we don't really have infrastructure for that yet. |
Most of the users have <500 scenarios per project. |
IIRC, you mentioned earlier that the expression in the StepDefinitionBinding was, in the case of Cucumber expressions, a Regex created by transforming the Cucumber expr into a Regex. Would displaying the regex in that situation be confusing for the user (as it doesn't match their own code)? |
There are two properties on the loaded step deft: the calculated regex and the original expression that the user entered. |
OK. It looks like if I use stepDefinition.Expression then I should be safe. |
Looks good! I like it! |
Pushed another commit. This includes testing via Specs. Fixed up unit tests (such as they are). I tested this with an external assembly and it worked fine. What other refactoring/clean-up would you like to see? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this looks all good. I have listed by review comments. If something is unclear, please let me know.
Have you done at the end some perf/ux testing with a larger project?
asyncContextMenu.AddItems(new ContextMenuItem("Could not complete find operation because of an error")); | ||
else if (summary.UnusedStepDefinitions == 0) | ||
asyncContextMenu.AddItems( | ||
new ContextMenuItem("Could not find any unused step definitions at the current position")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the message should be "Could not find any unused step definitions in the current project"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the messaging when there are no Unbound StepDefs. This feature scans all the projects in the solution, including external binding assemblies. At this point in the execution, all the work is done (all projects and assemblies scanned).
I would suggest:
"There are no unused step definitions" or perhaps "There are no unused step definitions in the solution"
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not exactly. It takes the binding registry from the current project. It scans all projects for feature files, but not for the step definitions. At least I think it is like that.
//TODO: determine if this required/needed | ||
//else if (summary.UsagesFound == 0) asyncContextMenu.AddItems(new ContextMenuItem("Could not find any usage")); | ||
|
||
//TODO: modify monitoring to include finding unused step definitions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would definitely include the monitoring, so that we can see how users will use this feature. You can make a new telemetry event similar to the find usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Added MonitorCommandFindUnusedSStepDefinitions to IMonitoringService and its two implementations (MonitoringService and NullVsIdeScope.cs)
else if (summary.UnusedStepDefinitions == 0) | ||
asyncContextMenu.AddItems( | ||
new ContextMenuItem("Could not find any unused step definitions at the current position")); | ||
//TODO: determine if this required/needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would include the not found message (with a different text of course)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
if (cancellationToken.IsCancellationRequested) | ||
break; | ||
|
||
var stepDefinitions = await GetStepDefinitionsAsync(project, fileName, triggerPoint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need to collect step definitions per file, because we can use the step definitions for the project (see comment below). So this few lines and the related methods (GetStepDefinitionsAsync
, GetStepDefinitions
) are not needed. Consequently you will not need the fileName
parameter either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored per your suggestion.
|
||
//await Task.Delay(500); | ||
|
||
//TODO: create menu item - what type of menu item? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SourceLocationContextMenuItem
you have used is fine, you can remove this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| Steps.cs(11,1): [When("I press multiply")] MyProject.CalculatorSteps.WhenIPressMultiply | | ||
And invoking the first item from the jump list navigates to the "I press multiply" "When" step definition | ||
|
||
Scenario: Find unused step definition with multiple step kind attributes, ignoring bound steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Scenario: Only finds unused step definition attributes when the method had multiple attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| Steps.cs(13,1): [Then("I press equals")] MyProject.CalculatorSteps.WhenIPressMultiply | | ||
And invoking the first item from the jump list navigates to the "I press multiply" "When" step definition | ||
|
||
Scenario: Find unused step definition Command reports properly when No Unused Binding Methods exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe Scenario: Reports if there were no unused step definitions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -6,6 +6,9 @@ | |||
<LangVersion>latest</LangVersion> | |||
<NoWarn>VSTHRD200<!--Use "Async" suffix in names of methods that return an awaitable type.--></NoWarn> | |||
</PropertyGroup> | |||
<ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All changes in this project file should be undone, these are done by visual studio by mistake. They are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
new StubBufferTagAggregatorFactoryService(tp), tp), "???") | ||
{ | ||
} | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
.HaveCount(2).And | ||
.Contain(mi => mi.Label == "Steps.cs(10,9): [When(\"I choose add\")] WhenIPressAdd"); | ||
} | ||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@gasparnagy thanks for the extensive feedback. I will get to work on those items.
That number can't possibly be correct; remember there were only 1,400 step definitions in the project. You might be able to help me by explaining how the generator works - how many step definitions should be expected to match with scenario steps? In a spot check, I see some that do and some that don't. If the generator is deterministic, I might be able to determine which types of step defs won't match and would then look at the logic of the command for bugs related to those types of step defs. |
@clrudolphi Ah. These are good questions. I haven't checked the generator for some years, so I don't really remember. One thing is for sure that it is deterministic. I will refresh my memories next week and give some hints. For the size: The 235 step definition classes and 1400 step definition methods look too much. I have no idea why it generated that many for 50 feature files. I will check. Whatever we measure, we should also compare with other features, like the "find step definition usages". If that one also hangs, the issue is not with your code (or maybe we need to include a few "yield" calls to give a chance for the UI to refresh). |
Refactored per your review suggestions. Open issues are:
|
The "Find step definition usages" operates quickly without hanging. |
Found a bug that may explain the weird results seen in the perf test. For each Project/FeatureFile, the algo subtracts those step defs that are matched and creates a set of "unused" from the remaining. The problem is that the set of step defs spans the entire solution, so stepDefs that don't match for a given feature file are reported as "unused" even when they might match a different feature file. Each stepDef is getting found as "unused" multiple times, at least as many as the number of featureFiles minus 1. |
I think I have this fixed. |
…Def in the project that didn't appear in the FF currently being examined. Minor modification to the logging to report back the number of Unused and the number of Projects (rather than the number of Features).
Nice! This is yet another good example of the importance of exploratory testing. We did specs, we did unit tests, we did review - these are good, but they all driven by our own thinking, so they cannot detect such out of the box problems. It's good that you went after the strange perf test results! |
I have checked it and indeed now it finds the unused step definitions from all, but in my opinion it is not convenient this way (I tested it in a real project and it found some totally unimportant unused one in an included sample). But I'm not 100% sure. The reason that it works this way is because you ask for the binding registry from the projects of the feature files here. If you would move up this call, and would only get the binding registry from the project of the editor in here (and pass down the binding registry) then it would only show the ones from the editor, I think. |
I've tried that, but it resulted in a bug of duplicate and erroneous findings. The reason was because the list of projects is being enumerated in PreExec resulting in all feature files (and scenarios) for all projects in the solution. When the StepDefs for the current project are compared against that list of scenario steps, the results are incorrect as the SDs are not matching against scenarios from other projects. Instead, the approach I took was to eliminate the enumeration of all project files, but simply used the current Project as the source of both SDs (via the Binding Registry) and scenarios (via the feature file loading and parsing done in FindUnused and FindUsagesFromContent). |
Updated the original PR form to reflect that Tests are now included. Before those get done, is there anything else code-wise or logic yet to do? |
I think I have confused you now (sorry) and I don't know what is the best option. Let me explain the complexity that causes the problem, because this is going to be useful info anyway. In the majority of the cases, the step definitions are in the same project as the feature files, but this is not always like that. You can include step definitions of another project as well with the Bindings from External Assemblies feature. You can include bindings from other projects that also have feature files (the projects that have feature files are called the ReqnrollTestProjects) or you can include bindings from projects that are just used to implement shared step definitions (these are called ReqnrollProjects). These ReqnrollProjects that are not ReqnrollTestProjects cannot have a Actually you can only get the binding registry of ReqnrollTestProjects. The binding registry of these projects will contain the step definitions of the project itself and also the step definitions of the used external step definitions as well. Technically you can probably get the binding registry of a ReqnrollProject that is not a ReqnrollTestProjects as well, but that is not really a valid operation (they might be dependent on some plugins or config settings that is only available in ReqnrollTestProjects). This altogether means that if we would like to list all step definitions in the current project that are unused, we cannot get the editor's binding registry as I suggested earlier, because that might be just a normal ReqnrollProject (without feature files), but we would need to enumerate all ReqnrollTestProjects that "use" the current project as an external binding source, get the unused step definitions of them and filter out these unused step definitions to show only the ones from the current project. Pretty complicated. (I think there is no function to list ReqnrollTestProjects that "use" the current project, but of course we could just list all of them the filtering will anyway ignore all their unused step definitions if they don't "use" the current project.) I'm not sure that we want this. The alternative is to revert the last change and rater implement this feature in a way that it finds all step definitions from all projects (as you did it anyway). The most of the users have anyway only one Reqnroll project, and we can still change this feature later to filter for the current project's step definitions if we will see big problems with this approach. Sorry again for misleading you... |
We don't yet have a documentation of the VS integration, so you cannot extend that (OK for now), but please add a line to the change log. I don't see anything else to be done. (Except solving the confusion that I caused as described above.) |
I'd like to ensure I correctly understand the nuances of what you've explained and ultimately the requirements for this feature.
|
Essentially your understanding is correct, my thoughts on the expected result only differs in the "ExternalAssembly" case, but that is something we haven't discussed anyway and the difference is not important. I have tried to rephrase the specs in a way that it is better fitting to my way of thinking. Instead of using the "sibling" term I simply used project "A" and "B" and added an extra condition whether the B is listed as additional binding assembly for A or not. This is what I got. As I said at the end the result is the same, the only diff is marked with
|
…rrent project (the project of the current editor). Added same modification as in GH#7 so that this command works within SpecFlow projects. Added CHANGELOG entry for this new feature.
@gasparnagy I've modified the Command to limit the set of UnUsed SDs to those within the current project. In doing so, I made a change to VsProjectScope.GetProjectFiles(string extension). While that method would take any string as an extension, it did not use it and instead was only returning feature files. I modified it such that it used the extension in the filtering process it ran. Let me know if that change is too intrusive or risky. I am a noob with git and not quite sure how to proceed here. I've sync'd my fork on GH to bring in the other changes that have been merged, and pulled them down to my machine. I then rebased these changes from main. My GH Desktop app is now reporting that my branch has 7 commits to push (6 from me of the work done already, which seems to me that it duplicates what has already been pushed to GH; and one that is a merge commit ("Merge branch 'reqnroll:main' into RNR70_UnusedStepDefinitions {the branch name}). that merge commit seems suspicious to me too. What did I do wrong? Any suggestions on how to proceed? I am guessing that a squash is needed. Should I have done that before rebasing? |
Good idea! I like it.
Nothing. This is good. When GH runs the PR checks, it automatically tries to merge your changes to the reqnroll:main and runs the tests on the merged result. So basically it checks the "merge compatibility" or "integration". If this succeeds, there is nothing else to do. In some cases your changes cannot be merged with the reqnroll:main because there has been changes there that are not compatible. This happens for example when you add a new line to the changelog while others have also added some, so git cannot decide which line should go first. In this cases, you need to "resolve the conflict" on the PR. This can be done in multiple ways. It could be done by rebasing the entire PR to reqnroll:main and force push, but it is not practical if people have done reviews (as we discussed earlier). The better option is to simply merge the reqnroll:main into your PR branch. (This might look ugly, because all the changes others have done will appear in this PR, but don't worry GH and git knows that those are not belong to this PR.) This is exactly what you did, so all ok. You do not need to do squashing, this will be done by GH once I "merge" the PR. Now as you are part of the core-team, it will be a bit easier, because you don't need to use & sync your own fork, you can work on the main repo directly: in the cloned main repo, just create a new branch and push it - on the GH Reqnroll project page it will ask you to make that a PR. Side note: As you work with "draft" PRs now, at the point where you think the PR is OK from your point of view, you can click on the "Ready for review" button. This is just an signal for the potential reviewers, does not really change anything. (I did that now.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great contribution, thanks for your efforts @clrudolphi.
This commit duplicates code from FindStepDefinitionUsageCommand. Not tested.
This is a preliminary PR to track the POC for adding a feature to the Reqnroll.VisualStudio extension that provides a context menu command that will find all StepDefinitions in the current project that are not bound to a feature file Step.
Checklist: